Skip to content

feat(sort): change sort priority indicator to only show when multiple columns are sorted #4876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 14, 2016

Conversation

jbarrus
Copy link
Contributor

@jbarrus jbarrus commented Dec 17, 2015

Sort priority number indicators were added with #4257. These look strange when only one column is sorted. This pull request changes the indicators to only display when multiple columns are sorted.

fixes #4503

@JLLeitschuh
Copy link
Contributor

This should be toggleable with a setting. Also this will conflict with #4807 (but I guess I'll have to take care of that)

@jbarrus jbarrus force-pushed the smart-hide-sort-priority branch from 8cdfbbe to 8a544e8 Compare December 19, 2015 21:36
@jbarrus
Copy link
Contributor Author

jbarrus commented Dec 19, 2015

What should the name of the setting be? hideSingleColumnSortIndicator?

I'm still having a hard time seeing why anyone would turn this off. Are there any other examples of grids that show numeric sort indicators like this when only one column is sorted?

@swalters
Copy link
Contributor

@JLLeitschuh I agree with the comment above. Is there a reason we need to always show a sort indicator of 1 if there is only one column sorted?

change sort priority indicator to only show when multiple columns are sorted
@jbarrus jbarrus force-pushed the smart-hide-sort-priority branch from 67247c0 to 7725eac Compare January 11, 2016 22:23
@jbarrus
Copy link
Contributor Author

jbarrus commented Jan 11, 2016

So does someone need to make a decision?

@dlgski
Copy link
Contributor

dlgski commented Jan 14, 2016

I like this idea. If more than 1 sort is provided, add numbers, (indexing at 1) otherwise do not show priority

@JLLeitschuh
Copy link
Contributor

Okay, this makes sense. I see no problem with merging this.

JLLeitschuh added a commit that referenced this pull request Jan 14, 2016
feat(sort): change sort priority indicator to only show when multiple columns are sorted
@JLLeitschuh JLLeitschuh merged commit 271863c into angular-ui:master Jan 14, 2016
@dlgski
Copy link
Contributor

dlgski commented Jan 20, 2016

There is something off with this solution. I just pulled 3.1.0 down to test this.

  1. I sort on 1 column --> I get an arrow indicator with no priority number shown (GOOD)
  2. I sort on a 2nd column --> First sort remains with same indicator, new column gets an arrow indicatory (BAD) ==> expected result: once I sort on a 2nd column I should see a "1" next to my first column and a "2" next to my second
  3. I sort on a 3rd column --> First column remains with no priority indicator, second column gets a "1" indicator, and 3rd column gets a "2" indicator (BAD) ===> expected result: I should see sort priorities "1", "2", "3"

@dlgski
Copy link
Contributor

dlgski commented Jan 20, 2016

The reason this is not working properly is because in ui-grid-header-cell.js you are testing for:

$scope.grid.columns.some(function(element, index){
                  return element.sort.priority !== undefined && element !== $scope.col;
                });

but if element.sort.priority === 0 then this condition is returning 0 which will result with isSortPriorityVisible resolving as 0 or "false" when it should be "true"

@dlgski
Copy link
Contributor

dlgski commented Jan 20, 2016

@JLLeitschuh where is your fix to index sort.priority at 1 instead of 0? If this went in, then the condition above would resolve to true and there would be no bug

bartkiewicz pushed a commit to bartkiewicz/ui-grid that referenced this pull request Jan 25, 2016
feat(sort): change sort priority indicator to only show when multiple columns are sorted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort Priority Numbers
4 participants